Add fallback implementation of PyCriticalSection_BeginMutex for Python 3.13t
#5981
Conversation
include/pybind11/detail/internals.h
Outdated
| # if defined(PY_VERSION_HEX) && PY_VERSION_HEX >= 0x030E00C1 // 3.14.0rc1 | ||
| PyCriticalSection_BeginMutex(&cs, &mutex.mutex); | ||
| # else | ||
| _PyCriticalSection_BeginMutex(_PyThreadState_GET(), &cs, &mutex.mutex); |
There was a problem hiding this comment.
I don't think this will compile on 3.13 without additional changes. _PyCriticalSection_BeginMutex is in an inline function in an internal-only header (pycore_critical_section.h).
I think we should use PyAPI_FUNC(void) _PyCriticalSection_BeginSlow(PyCriticalSection *c, PyMutex *m); instead. It's not inline, but you'll need to declare it above.
There was a problem hiding this comment.
Also, we'll still have the race condition with py::make_key_iterator in 3.13 due to the critical section implementation behaving differently in 3.13, but otherwise it should preserve compatibility.
There was a problem hiding this comment.
Also, we'll still have the race condition with
py::make_key_iteratorin 3.13 due to the critical section implementation behaving differently in 3.13, but otherwise it should preserve compatibility.
I think that's OK. We just want to limp through enough to make the pybind11 v3.0.2 release without breaking what worked with Python 3.13t before.
XuehaiPan
changed the title
PyCriticalSection_BeginMutex for Python 3.13tPyCriticalSection_BeginMutex for Python 3.13t
`_PyCriticalSection_BeginSlow` is a private CPython function not exported on Linux. For Python < 3.14.0rc1, use direct `mutex.lock()`/`mutex.unlock()` instead of critical section APIs.
|
I think this hangs. I think we probably should do nothing on 3.13t, rather than trying to also have a lock? |
|
What was the issue with |
Refactor pycritical_section into a unified class with internal version checks instead of using a type alias fallback. Skip locking in make_iterator_impl for Python < 3.14.0rc1 to avoid deadlock during type registration, as pycritical_section cannot release the mutex during Python callbacks without PyCriticalSection_BeginMutex.
There is a linkage issue on Linux. |
tests/test_multiple_interpreters.py
Outdated
| @pytest.mark.skipif( | ||
| sys.platform.startswith("emscripten"), reason="Requires loadable modules" | ||
| ) | ||
| @pytest.mark.xfail(env.MUSLLINUX, reason="Flaky on musllinux", strict=False) |
There was a problem hiding this comment.
Is this a known errata with the current fix?
There was a problem hiding this comment.
@XuehaiPan could you please add the URL to the reason?
|
You would need to copy the forward declaration from Python ( But if you get the mutex working, that seems fine too |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
@colesbury @XuehaiPan question about this comment:
I'm not sure if that means we should still do something, or if we're fine with this PR as-is? (it looks good to me, with my rather superficial specific background)
tests/test_multiple_interpreters.py
Outdated
| @pytest.mark.skipif( | ||
| sys.platform.startswith("emscripten"), reason="Requires loadable modules" | ||
| ) | ||
| @pytest.mark.xfail(env.MUSLLINUX, reason="Flaky on musllinux", strict=False) |
There was a problem hiding this comment.
@XuehaiPan could you please add the URL to the reason?
Neither $ cmake --build build --parallel --target pytest
...
[100%] Built target pybind11_tests
Traceback (most recent call last):
File "/home/PanXuehai/Projects/pybind11/tests/conftest.py", line 26, in <module>
import pybind11_tests
ImportError: /home/PanXuehai/Projects/pybind11/build/tests/pybind11_tests.cpython-313t-x86_64-linux-gnu.so: undefined symbol: _ZN8pybind116detail28_PyCriticalSection_BeginSlowEP17PyCriticalSectionP7PyMutex
ImportError while loading conftest '/home/PanXuehai/Projects/pybind11/tests/conftest.py'.
../../tests/conftest.py:26: in <module>
import pybind11_tests
E ImportError: /home/PanXuehai/Projects/pybind11/build/tests/pybind11_tests.cpython-313t-x86_64-linux-gnu.so: undefined symbol: _ZN8pybind116detail28_PyCriticalSection_BeginSlowEP17PyCriticalSectionP7PyMutex
gmake[3]: *** [tests/CMakeFiles/pytest.dir/build.make:76: tests/CMakeFiles/pytest] Error 4
gmake[2]: *** [CMakeFiles/Makefile2:601: tests/CMakeFiles/pytest.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:608: tests/CMakeFiles/pytest.dir/rule] Error 2
gmake: *** [Makefile:281: pytest] Error 2 |
rwgk
left a comment
rwgk
left a comment
There was a problem hiding this comment.
I looked at the changes myself, and with Cursor. This is a great pragmatic response to our current situation. I'll merge this PR now.
5 participants
Description
Fix #5971 (comment)
Suggested changelog entry: